Skip to content

Fix #2104: Instantiate unapply result type variables #3905

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jan 25, 2018

Conversation

odersky
Copy link
Contributor

@odersky odersky commented Jan 24, 2018

Type variables in an unapply result should be fully instantiated before nested
pattern matches. This is analogous to instantiating the scrutinee type of a match.

Type variables in an unapply result should be fully instantiated before nested
pattern matches. This is analogous to instantiating the scrutinee type of a match.
@odersky odersky requested a review from liufengyun January 24, 2018 13:47
@@ -811,8 +811,7 @@ class ErrorMessagesTests extends ErrorMessagesTest {
.expect { (ictx, messages) =>
implicit val ctx: Context = ictx

assertMessageCount(1, messages)
val AnonymousFunctionMissingParamType(param, args, _, pt) :: Nil = messages
val AnonymousFunctionMissingParamType(param, args, _, pt) = messages.last
Copy link
Contributor Author

@odersky odersky Jan 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is very mysterious: If I copy out the code of the test I always see 4 messages - one "missing param type", followed by 3 "cannot resolve overloading". that happens no matter whether I am on master or this PR.

On master the ErrorMessageTests succeeded, i.e. they report only one failure instead of the 4 we see in the terminal. On this PR this is no longer the case. Instead, it reports all 4 tests in messages, but in reverse order.

I have no idea what could be the cause of these effects.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reporter for the error message tests is mixed in with dotty.tools.dotc.reporting.UniqueMessagePositions which suppresses multiple error messages per position. This is why only one error message is reported. This PR must have changed the position of the reported error messages, so that now the test sees 4 messages as well.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is that in master, there is no overlap in positions of the 4 errors, and this PR doesn't change positions of the errors, and the order doesn't change either.

Copy link
Contributor

@liufengyun liufengyun Jan 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The root cause is the following line in Inferencing.scala (https://github.com/lampepfl/dotty/blob/master/compiler/src/dotty/tools/dotc/typer/Inferencing.scala#L339-L342):

      val hasUnreportedErrors = ctx.typerState.reporter match {
        case r: StoreReporter if r.hasErrors => true
        case _ => false
      }

      // ....

      if (!hasUnreportedErrors)
        vs foreachBinding { (tvar, v) =>
          if (v != 0 && ctx.typerState.constraint.contains(tvar)) {
            // previous interpolations could have already instantiated `tvar`
            // through unification, that's why we have to check again whether `tvar`
            // is contained in the current constraint.
            typr.println(s"interpolate ${if (v == 1) "co" else "contra"}variant ${tvar.show} in ${tp.show}")
            ensureConstrained()
            tvar.instantiate(fromBelow = v == 1)
          }
        }

In normal command line compilation, we are using ConsoleReporter, hasUnreportedErrors will be false, despite there's error in ConsoleReporter ( unspecified type of anonymous function), thus resulting instantiating of List.unapplySeq[A](null) with A = Nothing. The instantiation in turn leads to 3 overloading errors.

In the testing, we are using StoreReporter, thus hasUnreportedErrors will be true due to the unspecified type of the anonymous function. Now, instantiation of A in List.unapplySeq[A](null) will be delayed, after type checking (1, 2, 3). So there will not be overloading errors and A = Int eventually.

After the change in this PR, we are triggering early instantiation even for StoreReporter, thus now in test we get 4 errors.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for clearing this up, Fengyung!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to change or document hasUnreportedErrors somehow? The fact that different reporters will influence type inference doesn't sound correct.

@@ -920,6 +920,7 @@ trait Applications extends Compatibility { self: Typer with Dynamic =>
val ownType =
if (selType <:< unapplyArgType) {
unapp.println(i"case 1 $unapplyArgType ${ctx.typerState.constraint}")
fullyDefinedType(unapplyArgType, "pattern selector", tree.pos)
Copy link
Contributor

@liufengyun liufengyun Jan 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the diagnosis, the type parameter could be further constrained by patterns, thus I'm not sure if this change is the right fix. However, given that there is no test case invalidated this change, maybe we can accept the fix for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think it is analogous to fully instantiating the match scrutinee. In pattern matching all information flows from the scrutinee via the expected type to the pattern, never the other way round.

@liufengyun liufengyun assigned odersky and unassigned liufengyun Jan 24, 2018
@odersky odersky merged commit 1f51376 into scala:master Jan 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants